Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: race condition #686

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

itsUndefined
Copy link
Contributor

@itsUndefined itsUndefined commented Jan 30, 2023

Fix race condition when render/clear is called in rapid succession. This issue was first spotted when trying to use this library in react:
Relevant issue : briosheje/react-html5-qrcode-reader#2

Closes:
#668
#641
#500

@@ -324,6 +326,8 @@ export class Html5QrcodeScanner {
* fails otherwise.
*/
public clear(): Promise<void> {
this.isClearing = true;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you reset this state when clear() is done? Like in line 342?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but it doesn't fix the issue. Let me explain why:

In react 18 development mode, useEffect runs twice to make sure that the component is resilient to unmounting and remounting.

This means that in react this library receives these calls in rapid succession:

render() // First render
clear() // Clear is called and finishes before the first render is complete
render() // Second render

Because the clear function is called and is done executing before the first render is completed, there will be two rendered QR code scanners by the end of these calls. We need to pretty much cancel the first render completely when clear is called. That means that the isClearing variable should be reset to false only if another render is called.

I think a better naming for the variable is: isCleared. I renamed it.

Copy link
Owner

@mebjas mebjas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, added a comment.

Fix race condition when render/clear is called in rapid succession. This issue was first spotted when trying to use this library in react:
Relevant issue : briosheje/react-html5-qrcode-reader#2
@kaushalyap
Copy link

@mebjas can we please get this merged, this issue is really annoying.

Related: briosheje/react-html5-qrcode-reader#2

@dsd-dbeaulieu
Copy link

@mebjas this is still an issue, can we get this merged....?

@tuukka-miettinen
Copy link

@mebjas, this issue is still open and seems like the issues you raised have been dealt with.

Copy link

@nl-juntos-timon nl-juntos-timon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot test if it works, but code-wise it looks ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants